GITHUB#14399: decide index-sort early termination per segment in TopFieldCollector - #16434
Conversation
…ieldCollector TopFieldCollector cached whether the search sort is a prefix of the index sort after inspecting only the first segment. IndexWriter enforces a single index sort per index, but a MultiReader can combine indexes with different index sorts, so the cached decision is wrong for later segments and can drop results that should rank first. Decide this per segment in each TopFieldLeafCollector instead of caching it. disableSkipping() is added as a default method on LeafFieldComparator and is now called on the per-segment leaf comparators (NumericComparator and TermOrdValComparator drop their competitive iterator) rather than on the shared FieldComparator. Adds a test over a MultiReader whose two indexes are sorted in opposite directions; it fails before the fix (returns a non-competitive value) and passes after. Signed-off-by: Serhiy Bzhezytskyy <me@serhiy-bzhezytskyy.com>
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
This looks great, thanks @serhiy-bzhezytskyy!
| // Whether the search sort is a prefix of the index sort is decided per segment: a MultiReader | ||
| // may combine segments with different index sorts, so this cannot be cached across leaves | ||
| // (GITHUB#14399). | ||
| final Sort indexSort = context.reader().getMetaData().sort(); |
There was a problem hiding this comment.
It would be nice to use Sort.getPrimarySortField() here but I think that's going to end up being fairly complex in its interaction with sort prefixes, so we can leave that for a follow-up.
There was a problem hiding this comment.
Agreed on leaving it for a follow-up. canEarlyTerminate compares the full prefix, so a primary-field shortcut would have to keep the same answer for multi-field sorts — worth its own change with its own tests.
There was a problem hiding this comment.
Measured it since: substituting the shortcut into canEarlyTerminateOnPrefix returns a wrong result — an index sorted (a, c) searched by (a, b) gives the wrong top hit, because documents with equal a are ordered by c so their b order is arbitrary. testCanEarlyTerminateOnPrefix already asserts false for that shape. So the follow-up isn't needed here — the full prefix comparison is load-bearing.
| // calls disableSkipping(). | ||
| Directory dirA = newDirectory(); | ||
| IndexWriterConfig iwcA = newIndexWriterConfig().setIndexSort(ascSort); | ||
| iwcA.setMergeScheduler(new SerialMergeScheduler()); |
There was a problem hiding this comment.
Do we need to set the merge scheduler if we're force-merging down to a single segment?
There was a problem hiding this comment.
No — removed. forceMerge(1) is synchronous so it makes no difference: 50 randomized iterations pass without it, and it still fails on unmodified main.
forceMerge(1) is synchronous, so the scheduler makes no difference here. Removed after checking both directions: 50 randomized iterations pass without it, and it still fails on unmodified main with 'the smallest value (1, trailing docid of leaf B) must win expected:<1> but was:<50>'. The import stays: the pre-existing test at line 82 uses it. Signed-off-by: Serhiy Bzhezytskyy <me@serhiy-bzhezytskyy.com>
|
This is backwards-compatible, I think? So we can move the CHANGES entry to 10.6 and I will backport after merging. |
|
Yes — additive only. |
3 participants
Fixes #14399.
TopFieldCollectordecided whether the search sort is a prefix of the index sort by inspecting only the first segment, then cached that decision for the whole search:The premise holds for a single index —
IndexWriterenforces one index sort — but not for aMultiReader, which can combine indexes sorted differently. The cached decision is then wrong for later segments, and early termination drops documents that should have ranked first.This is a wrong-results bug, not only a missed optimisation. The added test builds a
MultiReaderover two indexes sorted in opposite directions (ndvascending andndvdescending) and searches withndvascending. The competitive document sits at the end of the second segment's docid order, so the decision cached from the first segment early-terminates the second one and discards it. Without the fix:The fix
Decide it per segment, inside
TopFieldLeafCollector, rather than caching across leaves.That change requires one adjustment: the old code called
disableSkipping()on the sharedFieldComparator, which is the wrong scope once the decision is per segment.disableSkipping()is now a default method onLeafFieldComparatorand is called on that segment's leaf comparators;NumericComparatorandTermOrdValComparatoroverride it to drop their competitive iterator. This is close to what @jpountz suggested on the issue, except that the method moves toLeafFieldComparatorrather than being removed fromFieldComparator— the leaf is the object whose lifetime matches the decision.Verification
mainwith the assertion above, and passes with the fix. Checked both ways rather than only the passing direction.TestTopFieldCollector*,TestSortOptimization,TestSearchAfterand thecomparatorspackage pass../gradlew check -x testis green.